Repository navigation
feat(ContextMenu): createContextMenu and useContextMenu, a headless context menu (spec:AST-107 FR8) - #8693
feat(ContextMenu): createContextMenu and useContextMenu, a headless context menu (spec:AST-107 FR8)#8693vjeux wants to merge 2018 commits into
Conversation
…ads its end before the arena's reset) into meta#8336
The case walked ContextMenu, DropdownMenu, useLongPress and useMenuPress's imports and failed on the UIKit gestures table: it kept that table out of the menus' bundles, the lazy design the no-lazy-load ruling and direct imports retire. The long-press numbers keep one source, still pinned by the table, recognizer and hooks case and by the hooks reaching longPress.ts.
… watcher test's tail fix) into the input hold
…xposes no ontouchstart is taken once Chromium fixes ontouchstart when the page is created, so turning touch emulation on later sends Touch Events without it: the host reads Pointer Events for the finger and must not read its Touch Events too. A one-finger long press fails here, the finger tracked twice.
…from their pointer events install() reads a finger from its pointer events when the window has no ontouchstart, but the four touch handlers still took the Touch Events a browser sends anyway (Chromium after touch emulation turns on), tracking the finger twice, so a one-finger long press never recognized. Each touch handler now returns while usesTouchEvents is false, after noting the fingers down, as install()'s comment promised.
…ter events) into meta#8320
…ter events) into meta#8293
…ter events) into meta#8356
…ter events) into meta#8358
…n before the click guard internal/motion.ts (meta#8562) re-exports UIKIT_CLICK from the barrel, so a tree carrying both fails typecheck and the Storybook build since this pull request dropped it. The barrel keeps it, with the click guard's window, and the table of every gesture group still leaves it out. Fails until the barrel exports it.
…'s window This pull request dropped UIKIT_CLICK when the click guard took the stray-click window over, but importers written before it read the barrel's UIKIT_CLICK (internal/motion.ts in meta#8562 re-exports it), so a tree carrying both failed typecheck and the Storybook build. UIKIT_CLICK stays in the barrel, deprecated, with the guard's 400 ms (CLICK_GUARD_WINDOW_MS), and UIKIT_GESTURES still leaves it out.
…ter events) into meta#8360
…ter events) into the every-pointer host # Conflicts: # packages/core/src/gestures/__tests__/GestureHost.test.ts
…peat control owns only its hold - A pen held still requests through the hold, and the browser then fires the pen's own context menu event, during the hold or at its lift: a second request for one gesture. One request only, the hold's. - The engineering owner's ruling on meta#8670: a hold-repeat control inside a row owns only its hold; its right click and Shift+F10 still reach the row's context request. Today both are refused there. Both fail until the context request refuses the pen's echo and leaves a kept press only its hold.
…eat control owns only its hold - The context request refuses a pen's own menu event during its hold and within the menu echo after its lift, as it refuses a held finger's and a key's. (A pen's barrel button already starts no press, from meta#8665.) - A hold-repeat control inside a row keeps only its hold: the row's right click and Shift+F10 reach the request through it, as the engineering owner ruled.
… with no options a record does not need The engineering owner's ruling on meta#8670: cut the two-stage commitMs, requestMs and requestOnLift, and the allowableMovement and isPerAxis tolerance, until a record calls for them. The hold is UIKit's long press, 500 ms and 10 px as a distance, and requests as it begins; onHold reports it committed, then ended or cancelled. LongPressGestureRecognizer loses isPerAxis and geometry its axisTravel, with their tests.
…, with no stages (spec:AST-094 FR43) Red: meta#8670 cuts the context request's two-stage hold (commitMs, requestMs, requestOnLift) and its per-axis allowance, as asked there: the hold is UIKit's long press and requests as it begins. ContextMenu and the factory's menu still open a held finger at 735 ms, open at a lift after 400 ms, and allow 11.5 pt on each axis. They should open at 500 ms under any primary pointer, open nothing for a lift before it, and open nothing for a drift past 10 pt.
…press, with no options) into the headless context menu packages/core/src/utils/pressFeedback.ts and its test: meta#8670's text, with this stack's press-paint subscription (subscribePressPaint, which ContextMenu's press feedback follows) kept beside it. packages/core/src/hooks/useLongPress.ts, its test, its page and useMenuPress.ts: meta#8670's text. This stack's useLongPress `stages` and press start times served ContextMenu's two-stage hold, which no longer exists (the context request opens ContextMenu, and its hold has no stages), so they go, with their changeset and the hooks entry's LongPressStage type.
… with none of the hold options meta#8670 cut (spec:AST-094 FR43) meta#8670 cuts the context request's two-stage hold (commitMs, requestMs, requestOnLift), allowableMovement and isPerAxis, as asked there: the hold is UIKit's long press and requests as it begins. ContextMenu and the factory passed them for UIContextMenuInteraction's 400 ms commit, 735 ms menu, open at a lift and 11.5 pt on each axis. They pass none now: under any primary pointer a held finger gets the menu at 500 ms, a lift before it is a tap, and a drift past 10 pt (a distance) opens nothing. Under the touch model the commit, now the long press, plays the haptic and the press feedback plays UIKit's press curve until the menu shows. The UIKit table's per-axis drift guess has no reader left and goes. The tests, the interaction specs, the docs, the story and the changesets that described the two-stage hold describe the long press; the tests of a lift after the commit and of a drag between the commit and the menu go with that window.
…ow, and Escape returns focus to it from another row's focus (spec:AST-094 FR33, spec:AST-107 FR8) The story's Inbox row, with Drafts focused first: a held finger opens its menu with the lift up beside it, and Escape closes the menu and focuses Inbox, in Chromium and WebKit.
…pec PR, as asked on meta#8693 architecture:layer-runtime INV13 (the exit copy), the three springs.contextMenu* rows in spec:AST-094 FR4, the spec:AST-057 reword and component:ContextMenu DEC-2 return to main's text here; a spec PR carries them for the owner's exact-head approval, so this code can land without waiting on them.
…ndle guard, and pressableCoverage's model files)
…int (spec:AST-107 FR6) Red: AST-107's public contract imports useContextMenu from @astryxdesign/core/hooks, which lacks it; asked on meta#8693.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on two spec points: the sentence this branch adds to spec:AST-094 FR41 (a mouse or pen drag that a recognizer claims suppresses text selection), which the current FR41 and its verification row do not allow, and ContextMenu's lift playing springs.contextMenuLift, springs.contextMenuDismiss and springs.contextMenuMorph as literals, which FR4 forbids until #8830 names them. An engineering owner, the author included, can record that decision by commenting /approve-spec <full head SHA>, or the FR41 edit can leave this pull request and the lift can wait for #8830. Whatever the decision, please land the stacked predecessors and rebase so this diff is the headless menu and ContextMenu's move onto it (about 46,000 changed lines today), and fix these:
ContextMenu.tsx:459keeps the menu slot's DOM node in React state, whichreact-component-runtimeINV1 forbids.- The every-commit effects at
ContextMenu.tsx:503anduseContextMenu.tsx:151should name the system they synchronize (INV3); ContextMenu's re-renders its menu whenever a caller passes a newitemsarray. ContextMenuContentRendererand the dense docs say therendercleanup runs at close, but it runs atdestroy.
[Automated review]
| // content on scroll (#3465); the menu places it. | ||
| const cursorAnchorRef = useRef<HTMLSpanElement>(null); | ||
| // The menu's container lives here, beside the trigger. | ||
| const [menuSlot, setMenuSlot] = useState<HTMLSpanElement | null>(null); |
There was a problem hiding this comment.
react-component-runtime INV1 says "DOM nodes and whole DOM collections MUST NOT be stored in React state; store only the minimal rendering key or own the node through a ref". This slot node enters state through ref={setMenuSlot} (line 573), so every ContextMenu, each Table cell with context actions included, renders a second time when the ref lands and a third when the effect at line 520 attaches the factory, and the trigger has no context request until that passive effect runs. Owning the slot through a ref, or letting the factory place its container from the trigger, avoids both.
| useIsomorphicLayoutEffect(() => { | ||
| onOpenChangeRef.current = onOpenChange; | ||
| latestRef.current = {factoryOptions, view}; | ||
| const controller = controllerRef.current; | ||
| if (controller == null) { | ||
| return; | ||
| } | ||
| updateContextMenu(controller, factoryOptions(slot)); | ||
| viewContextMenu(controller, { | ||
| ...view, | ||
| cursorAnchor: cursorAnchorRef.current, | ||
| containerParent: menuSlot, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This every-commit layout effect names no system it synchronizes (INV3: "A new or changed Effect MUST name the external system it synchronizes"), and through updateContextMenu/viewContextMenu it sets this component's menu state whenever the options differ. Table's lazy cell menu passes items={options ?? []} (Table/tableContextMenu.tsx:113), a new array on every render, so each render of such a cell renders its ContextMenu twice, the second synchronously from here. Naming the factory as the synchronized system, and comparing items by content before re-rendering the portal, would fix both.
| useLayoutEffect(() => { | ||
| latestRef.current = {options, wrap}; | ||
| const controller = controllerRef.current; | ||
| if (controller == null) { | ||
| return; | ||
| } | ||
| const portal = portalRef.current; | ||
| updateContextMenu( | ||
| controller, | ||
| factoryOptions(options, handleOpenChange, portal), | ||
| ); | ||
| bridgeContextMenu(controller, wrap); | ||
| if (portal != null) { | ||
| portal.draw(options.menuContent, wrap); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This every-commit layout effect passes options, providers and menuContent to the factory and to the hook's own root, but has no comment naming the system it synchronizes, which INV3 requires of a new Effect. A one-line comment naming the factory's menu and the hook's content root would settle it.
| /** | ||
| * Renders a menu's content into `container` without React; returns a cleanup | ||
| * that runs when the menu closes. | ||
| */ | ||
| export type ContextMenuContentRenderer = ( | ||
| container: HTMLElement, | ||
| ) => (() => void) | undefined; |
There was a problem hiding this comment.
This exported comment says the cleanup "runs when the menu closes", and createContextMenu.doc.mjs:138 says render "fills the menu in plain DOM when it opens". The content mounts with the closed menu, so render runs once as the menu is attached and its cleanup runs at destroy (lines 315-326, 403-405), as createContextMenu.doc.mjs:38 says. A caller who frees resources or rebuilds rows per open, as this comment suggests, sees neither happen.
| (`spec:AST-017` FR59; DEC-9). A mouse or pen drag that a recognizer claims | ||
| suppresses text selection for that drag only; an unclaimed mouse drag keeps | ||
| today's behavior. |
There was a problem hiding this comment.
This sentence adds a requirement the current FR41 does not state: on main, a mouse "otherwise keeps its released behavior", and the unchanged verification row still fails "a change ... to mouse, keyboard, or assistive-technology selection". It also leaves open what happens to a selection made before the drag, which the lock clears at the claim (packages/core/src/utils/selectionLock.ts:148). An engineering owner's exact-head decision is needed for this edit.
b9608e9 to
5e9e161
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: may the headless menu from createContextMenu and useContextMenu open under a held finger with no row lit, when spec:AST-107 FR8 says focus starts on the first enabled row? spec:AST-094 FR33 states the no-row rule only for ContextMenu. An engineering owner, the author included, can record the decision by commenting /approve-spec <full head SHA> or by amending FR8 on main.
Whatever the decision, these need changes:
- The branch conflicts with
mainin 101 files (the AST-094 record,ContextMenu.tsxand the gesture host among them), so please mergemainand keep only the work that has not landed. motion/uikit/springs.ts:502keepssprings.contextMenuLift,springs.contextMenuDismissandsprings.contextMenuMorphout of the role table, while FR4 onmainnames all three and forbids literals.- The sentence added to
spec:AST-094FR41 restates a rule that FR41 onmainalready states in other words, so it should be dropped. ContextMenu.tsx:459keeps the menu slot's DOM node in React state, whichreact-component-runtimeINV1 forbids.- The effects at
ContextMenu.tsx:503,ContextMenu.tsx:705anduseContextMenu.tsx:151synchronize no named external system (INV3); name it, or readattachment.elementdirectly in the second. ContextMenuContentRendererand the dense doc say therendercleanup runs at close or open, butrenderruns once and its cleanup runs atdestroy.- Neither API ships the live example that
spec:AST-107FR5 asks for, such as a template block naming it inexampleFor. - At 45,951 authored lines, the diff should shrink to the headless menu, ContextMenu's move onto it and the touch work that has not landed yet.
[Automated review]
| @@ -483,6 +499,8 @@ export const springs = { | |||
| /** | |||
| * The springs spec:AST-094 FR4 names: the only ones that are motion roles. | |||
| * The registry takes these rows alone, so registering ships no other spring. | |||
| * ContextMenu's lift, dismissal and morph springs stay internal constants | |||
There was a problem hiding this comment.
spec:AST-094 FR4 on main now names springs.contextMenuLift, springs.contextMenuDismiss and springs.contextMenuMorph, and says a component "MUST NOT play a literal". Add the three to SPRING_ROLES (as main does) and drop this comment; ContextMenuLift.tsx:131-133 already claims they resolve through their roles.
| (`spec:AST-017` FR59; DEC-9). A mouse or pen drag that a recognizer claims | ||
| suppresses text selection for that drag only; an unclaimed mouse drag keeps | ||
| today's behavior. |
There was a problem hiding this comment.
FR41 on main already states this rule in other words (a claimed drag "takes no new text selection for that drag and never clears an existing selection; a drag that starts on selectable text is not claimed"). Drop this sentence and keep main's when you merge.
| // content on scroll (#3465); the menu places it. | ||
| const cursorAnchorRef = useRef<HTMLSpanElement>(null); | ||
| // The menu's container lives here, beside the trigger. | ||
| const [menuSlot, setMenuSlot] = useState<HTMLSpanElement | null>(null); |
There was a problem hiding this comment.
architecture:react-component-runtime INV1: DOM nodes must not live in React state. Keep the slot in a ref and create the controller from the slot's ref callback.
| }; | ||
| const latestRef = useRef({factoryOptions, view}); | ||
|
|
||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
This every-commit effect names no external system (INV3). One line naming the factory's options and view as what it synchronizes would do.
| const layerAnchorIdRef = useRef(''); | ||
| const triggerRef = useRef<HTMLElement>(null); | ||
| // The headless context menu's element is the trigger. | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
This effect copies a prop into a ref and synchronizes no external system (INV3). Read attachment.element directly instead.
| latestRef.current.options.onOpenChange?.(isOpen); | ||
| }, []); | ||
|
|
||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
This every-commit effect names no external system (INV3); say it pushes the latest options and providers to the factory.
| /** | ||
| * Renders a menu's content into `container` without React; returns a cleanup | ||
| * that runs when the menu closes. | ||
| */ |
There was a problem hiding this comment.
render runs once as the menu is attached and its cleanup runs at destroy, not when the menu closes (see createContextMenu.test.tsx:285-307). Suggested: "Fills the menu's container once, as the menu is attached; returns a cleanup that runs at destroy."
| element: 'The element that owns the menu; its markup is untouched.', | ||
| 'options.items': "ContextMenu's rows. Give items or render.", | ||
| 'options.render': | ||
| '(container) => cleanup; fills the menu in plain DOM when it opens.', |
There was a problem hiding this comment.
Same timing issue: render fills the menu once at attach, not when it opens, and its cleanup runs at destroy.
…its menu in the element's dialog, popover or shadow root; its rows at the menu's layer depth; main dcdb356) into ContextMenu on the factory createContextMenu.tsx: meta#8693's container placement, rows context and wrapper access, with this branch's request core (attachContextMenuRequests) in place of the factory's own context request.
…pec; the factory closes on a press outside, mouse or finger) into ContextMenu on the factory createContextMenu.test.tsx: both sides add cases at its end; this keeps both, and the finger's outside-press cases take the file's shared clock epoch, since the touch-model cases before them have moved the gesture host's clock past a fresh 100 s.
…ng its roots without unmounting them inside React's commit; the factory's anchor through a transformed dialog) into ContextMenu on the factory createContextMenu.tsx: the wrapper access carries both this branch's view setter and meta#8693's destroy for React callers; teardown destroys this branch's request core. useContextMenu.tsx: the rows slot's host goes at once and its root renders nothing.
…pec PR, as asked on meta#8693 architecture:layer-runtime INV13 (the exit copy), the three springs.contextMenu* rows in spec:AST-094 FR4, the spec:AST-057 reword and component:ContextMenu DEC-2 return to main's text here; a spec PR carries them for the owner's exact-head approval, so this code can land without waiting on them.
…ndle guard, and pressableCoverage's model files)
…int (spec:AST-107 FR6) Red: AST-107's public contract imports useContextMenu from @astryxdesign/core/hooks, which lacks it; asked on meta#8693.
…ks entry (spec:AST-107 FR6) Red: ContextMenu's entry exports useContextMenu too, beside the hooks entry AST-107 imports it from; the other public gesture hooks are exported from the hooks entry alone. Asked in the API pass on meta#8693.
Carries #8125, #8201, #8213 and #8228 (ContextMenu's touch line) and #8670 (the context request); #8747 (ContextMenu built on the factory) is merged in, as asked. This head's own changes: vjeux/astryx@061a70f...b9608e9
User need
Part of the UIKit-parity program (
spec:AST-107FR8): a context menu on an element a product already renders, such as a list row, with or without React. ContextMenu and the factory are one implementation, so they open, present and dismiss alike.Problem and solution fit
createContextMenu(element, options)attaches ContextMenu's menu to the element; it opens on the element's context request (#8670) oropen(at), anddestroy()removes all it added.useContextMenu(ref, options), from@astryxdesign/core/hooks, is its React wrapper (FR6).<ContextMenu>attaches the factory to its own trigger (component:ContextMenuFR10). Under a finger the menu runs ContextMenu's touch model: the press feedback, the haptic and, on a block element, the lift (spec:AST-094FR33).What led to this push, from review:
open()while another control had focus, closing focused that control: focus now returns to the element.Current specification
spec:AST-107FR5, FR6 and FR8;component:ContextMenuFR10;spec:AST-094FR33 and FR43.Public delta and caller burden
New exports:
createContextMenuand its types, anduseContextMenufrom the root and/hooks. The element needs a tab stop for Shift+F10. Size (gzip): ContextMenu JS 177,734 B, CSS 15,189 B; it includesreact-dom/client, which the factory's own menu root imports. Public-API gaps (FR7): the lift, its springs and UIKit tables.Design and alternatives
Evidence
The steps below, automated in Chromium and WebKit; unit tests for focus return, the lift on a factory row, the long press's moment, the shadow root, the nested layer, a moved ref and an unmount that logs nothing.
Scope
Testing
In the story "Headless / createContextMenu on a list row":
menuContent), pick "Restore": it acts once and closes.pnpm test:interaction ContextMenu createContextMenuruns these. Actions are paused on the repository, so this head's results are local: unit 15,389, scripts 2,434, docsite 660, interaction 66 (Chromium) and 58 (WebKit), a11y 27; tsc, lint andpnpm check:repoclean.